Fixing and synchronizing dumper #118
Conversation
| /* see below | ||
| var isVolatile = new TypeReference("System.Runtime.CompilerServices", "IsVolatile", module, | ||
| module.TypeSystem.CoreLibrary); | ||
| */ |
There was a problem hiding this comment.
If this is no longer important, it should be removed.
|
|
||
| clone = | ||
| new MethodDefinition(original?.Name ?? "_" + md.Name.Replace(".", "_"), md.Attributes, module.TypeSystem.Void) | ||
| { | ||
| MethodReturnType = md.MethodReturnType, | ||
| Attributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Static, | ||
| Attributes = MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.Public | MethodAttributes.Static, |
There was a problem hiding this comment.
I don't see any reason to have Public included twice.
| ImplAttributes = MethodImplAttributes.IL | MethodImplAttributes.Managed, | ||
| DeclaringType = td, | ||
| HasThis = false | ||
| HasThis = false, | ||
| NoInlining = true |
There was a problem hiding this comment.
Dumps are broken without this when you try to dump methods that are inlined, or when dumping a transpiler that is postfixed, or situations like that. Same thing for relinker below. At least this is what this PR is supposed to fix, and it did when I tested it.
For context, this is what happened when I tried to dump a postfix that was inlined:
Original method:
public static NoteLineLayer GetNoteLineLayer(int lineLayer)
{
switch (lineLayer)
{
case 0:
return NoteLineLayer.Base;
case 1:
return NoteLineLayer.Upper;
case 2:
return NoteLineLayer.Top;
default:
return NoteLineLayer.Base;
}
}Postfix:
[HarmonyPatch(typeof(BeatmapDataLoaderVersion3.BeatmapDataLoader.ObstacleConverter), nameof(BeatmapDataLoaderVersion3.BeatmapDataLoader.ObstacleConverter.GetNoteLineLayer))]
internal class BeatmapDataLoaderObstacleConverterGetNoteLineLayerPatch
{
private static void Postfix(ref NoteLineLayer __result, int lineLayer)
{
if (lineLayer > 2)
{
__result = (NoteLineLayer)lineLayer;
}
}
}| }; | ||
|
|
There was a problem hiding this comment.
Avoid formatting changes wherever possible. It makes pull requests more annoying to review.
| /*if (instr.Previous?.OpCode == OpCodes.Volatile && | ||
| operand is FieldReference fref && | ||
| (fref.FieldType as RequiredModifierType)?.ModifierType != tr_IsVolatile) { | ||
| fref.FieldType = new RequiredModifierType(tr_IsVolatile, fref.FieldType); | ||
| }*/ |
There was a problem hiding this comment.
As above, if something isn't important, delete it.
| private static Instruction LabelFix(ILLabel label, Collection<Instruction> clone, Collection<Instruction> original) | ||
| { | ||
| var target = label.Target; | ||
| if (!clone.Contains(target)) | ||
| { | ||
| var idx = original.IndexOf(label.Target); | ||
| if (idx != -1) return clone[idx]; | ||
| } | ||
| return target; | ||
| } |
There was a problem hiding this comment.
Are clone and original guaranteed to have the same count? If so, add a Debug.Assert call. If not, clone[idx] is a bug.
Are there actually situations where clone.Contains(target) returns true?
I'm uncomfortable with ever returning target. I feel like it should throw if the instruction isn't found.
| Relinker relinker = (mtp, _) => | ||
| { | ||
| if (mtp == md) | ||
| return clone!; | ||
| if (mtp is MethodReference mr) | ||
| { | ||
| if (mr.FullName == md.FullName | ||
| && mr.DeclaringType.FullName == md.DeclaringType.FullName | ||
| && mr.DeclaringType.Scope.Name == md.DeclaringType.Scope.Name) | ||
| return clone!; | ||
| } | ||
| return module.ImportReference(mtp); | ||
| }; |
There was a problem hiding this comment.
Updated from reorg: https://github.com/MonoMod/MonoMod/blob/26a0a4d22fcd233dd22db4fd034cb3ac41420fd4/src/MonoMod.Utils/DMDGenerators/DMDCecilGenerator.cs#L72-L83
Which is also the same for the double Public, and everything else but the label stuff. Label stuff specifically addressed the issue I mentioned above.
|
While it fixed some of the cases I encountered previously, it seems to still be causing issues.
Patch: private static IEnumerable<CodeInstruction> Transpiler(IEnumerable<CodeInstruction> instructions)
{
// Replaces the obstacle width.
return new CodeMatcher(instructions)
.MatchEndForward(
new CodeMatch(i => i.opcode == OpCodes.Callvirt && ((MethodBase)i.operand).Name == $"get_{nameof(ObstacleController.width)}"),
new CodeMatch(OpCodes.Conv_R4),
new CodeMatch())
.ThrowIfInvalid()
.Insert(Transpilers.EmitDelegate<Func<float, float>>(obstacleWidth =>
{
if (!Plugin.active || obstacleWidth is < 1000 and > -1000)
{
return obstacleWidth;
}
if (obstacleWidth <= -1000)
{
obstacleWidth += 2000;
}
return (obstacleWidth - 1000) / 1000;
}))
.InstructionEnumeration();
}Original: public ObstacleSpawnData GetObstacleSpawnData(ObstacleData obstacleData)
{
Vector3 obstacleOffset = this.GetObstacleOffset(obstacleData.lineIndex, obstacleData.lineLayer);
obstacleOffset.y += this._jumpOffsetYProvider.jumpOffsetY;
obstacleOffset.y = Mathf.Max(obstacleOffset.y, this._verticalObstaclePosY);
float num = Mathf.Min((float)obstacleData.height * StaticBeatmapObjectSpawnMovementData.layerHeight, this._obstacleTopPosY - obstacleOffset.y);
float num2 = (float)obstacleData.width * 0.6f;
obstacleOffset.x += (num2 - 0.6f) * 0.5f;
return new ObstacleSpawnData(obstacleOffset, num2, num);
} |
|
@Meivyn If you wrote unit tests demonstrating your bug, that would be exceedingly useful and would significantly increase the likelihood that fixes get merged. |
|
It's actually because of This PR was opened after a conversation with them that started here: https://discord.com/channels/623153565053222947/1128639196169510942/1265164603151618129 |
No description provided.